Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(brotli) remove re2 dependency #1813

Merged
merged 1 commit into from
Aug 13, 2024
Merged

fix(brotli) remove re2 dependency #1813

merged 1 commit into from
Aug 13, 2024

Conversation

yigaldviri
Copy link
Contributor

Use a fixed version of re2 to maintain support for Node14 (now need 14.21.3)

Checklist

  • [V] I have ensured my pull request is not behind the main or master branch of the original repository.
  • [V] I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
  • [V] I have written a commit message that passes commitlint linting.
  • [V] I have ensured that my code changes pass linting tests.
  • [V] I have ensured that my code changes pass unit tests.
  • [V] I have described my pull request and the reasons for code changes along with context if necessary.

@socket-security Socket Security
Copy link

socket-security bot commented Aug 6, 2024 •
edited
Loading

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher

🚮 Removed packages: npm/[email protected]

View full report↗︎

@socket-security Socket Security
Copy link

socket-security bot commented Aug 6, 2024 •
edited
Loading

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

This was referenced Aug 6, 2024
Merged
Merged
@yigaldviri
Copy link
Contributor Author

Hi @titanism - can we merge it? It will help to keep support for Node 14 as well

@titanism
Copy link
Collaborator

It looks like this causes problems on some architectures, so we may need to look at an alternative.

#1814

At a glance, it seems like the regular expressions here are actually safe:

superagent/src/utils.js

Lines 121 to 139 in 134cdd7

/**
* Check if the response is compressed using Gzip or Deflate.
* @param {Object} res
* @return {Boolean}
*/
exports.isGzipOrDeflateEncoding = (res) => {
return new SafeRegExp(/^\s*(?:deflate|gzip)\s*$/).test(res.headers['content-encoding']);
};
/**
* Check if the response is compressed using Brotli.
* @param {Object} res
* @return {Boolean}
*/
exports.isBrotliEncoding = (res) => {
return new SafeRegExp(/^\s*(?:br)\s*$/).test(res.headers['content-encoding']);
}; 

> require('safe-regex2')(/^\s*(?:deflate|gzip)\s*$/)
true
> require('safe-regex2')(/^\s*(?:br)\s*$/)
true

So we may not need RE2 after all. Though this might need tested.

@titanism titanism mentioned this pull request Aug 10, 2024
Closed
3 tasks
@yigaldviri yigaldviri changed the title fix(brotli) use a fixed version of re2 to support Node 14 fix(brotli) remove re2 dependency Aug 11, 2024
@yigaldviri
Copy link
Contributor Author

@titanism - I removed re2. we have unit tests around that area

@psoleckimoj psoleckimoj mentioned this pull request Aug 12, 2024
Merged
@bizob2828 bizob2828 mentioned this pull request Aug 12, 2024
Closed
@titanism titanism merged commit edf0986 into ladjs:master Aug 13, 2024
5 checks passed
@titanism
Copy link
Collaborator

superagent v10.0.1 is released (we removed re2 dependency)

https://github.com/ladjs/superagent/releases/tag/v10.0.1

@yigaldviri yigaldviri deleted the re2 branch August 13, 2024 05:07
@jsrobertson jsrobertson mentioned this pull request Aug 14, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yigaldviri @titanism